Avoid the_repository in merge-ort and replay#2048
Open
newren wants to merge 6 commits intogitgitgadget:masterfrom
Open
Avoid the_repository in merge-ort and replay#2048newren wants to merge 6 commits intogitgitgadget:masterfrom
newren wants to merge 6 commits intogitgitgadget:masterfrom
Conversation
newren
force-pushed
the
avoid_the_repository
branch
from
e2edab9 to
d75a71a
Compare
Author
|
/submit |
|
Submitted as pull.2048.git.1771406115.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
User |
newren
force-pushed
the
avoid_the_repository
branch
from
d75a71a to
f9e2474
Compare
|
User |
|
User |
newren
force-pushed
the
avoid_the_repository
branch
2 times, most recently
from
6a78332 to
67db46f
Compare
Author
|
/submit |
|
Submitted as pull.2048.v2.git.1771552788.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
Prefetching of blobs from promisor remotes was added to diff in 7fbbcb2 (diff: batch fetching of missing blobs, 2019-04-05). In that commit, https://lore.kernel.org/git/20190405170934.20441-1-jonathantanmy@google.com/ was squashed into https://lore.kernel.org/git/44de02e584f449481e6fb00cf35d74adf0192e9d.1553895166.git.jonathantanmy@google.com/ without the extra explanation about the squashed changes being added to the commit message; in particular, this explanation from that first link is absent: > Also, prefetch only if the repository being diffed is the_repository > (because we do not support lazy fetching for any other repository > anyway). Then, later, this checking was spread from diff.c to diffcore-rename.c and diffcore-break.c by 95acf11 (diff: restrict when prefetching occurs, 2020-04-07) and then further split in d331dd3 (diffcore-rename: allow different missing_object_cb functions, 2021-06-22). I also copied the logic from prefetching blobs from diff.c to merge-ort.c in 2bff554 (merge-ort: add prefetching for content merges, 2021-06-22). The reason for all these checks was noted above -- we only supported lazy fetching for the_repository. However, that changed with ef830cc (promisor-remote: teach lazy-fetch in any repo, 2021-06-17), so these checks are now unnecessary. Remove them. Signed-off-by: Elijah Newren <newren@gmail.com>
In order to get rid of a usage of the_repository, we need to know the value of opt->repo; pass it along to write_tree(). Once we have the repository, though, we no longer need to pass opt->repo->hash_algo->rawsz, we can have write_tree() look up that value itself. Signed-off-by: Elijah Newren <newren@gmail.com>
newren
force-pushed
the
avoid_the_repository
branch
from
67db46f to
1b1b410
Compare
We have a perfectly valid repository available and do not need to use the_repository. Signed-off-by: Elijah Newren <newren@gmail.com>
We have a perfectly valid repository available and do not need to use the_hash_algo (a shorthand for the_repository->hash_algo), so use the known repository instead. Signed-off-by: Elijah Newren <newren@gmail.com>
Due to the use of DEFAULT_ABBREV, we cannot get rid of our usage of USE_THE_REPOSITORY_VARIABLE. However, we have removed all other uses of the_repository in merge-ort a few times. But they keep coming back. Define the_repository to make it a compilation error so that they don't come back any more. Signed-off-by: Elijah Newren <newren@gmail.com>
Due to the use of DEFAULT_ABBREV, we cannot get rid of our usage of USE_THE_REPOSITORY_VARIABLE. We have removed all other uses of the_repository before, but without removing that definition, they keep coming back. Define the_repository to make it a compilation error so that they don't come back any more; the repo parameter plumbed through the various functions can be used instead. Signed-off-by: Elijah Newren <newren@gmail.com>
newren
force-pushed
the
avoid_the_repository
branch
from
1b1b410 to
0654d04
Compare
Author
|
/submit |
|
Submitted as pull.2048.v3.git.1771718393.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Changes since v2:
>
> * In first patch, actually avoid the_repository when attempting to remove
> check against the_repository
> * Fix commit message of patch 3 due to the new patch 1.
> * Slight tweak to commit message of patch 6.
> ...
> As noted in the comments on v1, I actually do not know why
> prefetch_for_content_merges() needs to use the_repository. When I introduced
> it back in 2bff554b23e8 (merge-ort: add prefetching for content merges,
> 2021-06-22), I was just looking at diffcore_std() and trying to mimic how it
> did the prefetch, and it has such a comparison. If anyone knows why
> diffcore_std() needs to compare against the_repository, I'd love to hear...
Is this comment still current?
> Elijah Newren (6):
> merge,diff: remove the_repository check before prefetching blobs
> merge-ort: pass repository to write_tree()
> merge-ort: replace the_repository with opt->repo
> merge-ort: replace the_hash_algo with opt->repo->hash_algo
> merge-ort: prevent the_repository from coming back
> replay: prevent the_repository from coming back
I do not seem to see the last step on the list archive.
https://lore.kernel.org/git/pull.2048.v3.git.1771718393.gitgitgadget@gmail.com/
I'll resurrect it using the previous one and ...
> 6: 67db46f34f ! 6: 0654d04584 replay: prevent the_repository from coming back
> @@ Commit message
> coming back.
>
> Define the_repository to make it a compilation error so that they don't
> - come back any more.
> + come back any more; the repo parameter plumbed through the various
> + functions can be used instead.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
... this piece of information.
|
|
Elijah Newren wrote on the Git mailing list (how to reply to this email): On Sat, Feb 21, 2026 at 6:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > Changes since v2:
> >
> > * In first patch, actually avoid the_repository when attempting to remove
> > check against the_repository
> > * Fix commit message of patch 3 due to the new patch 1.
> > * Slight tweak to commit message of patch 6.
> > ...
> > As noted in the comments on v1, I actually do not know why
> > prefetch_for_content_merges() needs to use the_repository. When I introduced
> > it back in 2bff554b23e8 (merge-ort: add prefetching for content merges,
> > 2021-06-22), I was just looking at diffcore_std() and trying to mimic how it
> > did the prefetch, and it has such a comparison. If anyone knows why
> > diffcore_std() needs to compare against the_repository, I'd love to hear...
>
> Is this comment still current?
No, I should have pulled it out of the cover letter since the commit
message of patch #1 answers this; sorry for the oversight.
> > Elijah Newren (6):
> > merge,diff: remove the_repository check before prefetching blobs
> > merge-ort: pass repository to write_tree()
> > merge-ort: replace the_repository with opt->repo
> > merge-ort: replace the_hash_algo with opt->repo->hash_algo
> > merge-ort: prevent the_repository from coming back
> > replay: prevent the_repository from coming back
>
> I do not seem to see the last step on the list archive.
Weird.
> https://lore.kernel.org/git/pull.2048.v3.git.1771718393.gitgitgadget@gmail.com/
>
> I'll resurrect it using the previous one and ...
>
> > 6: 67db46f34f ! 6: 0654d04584 replay: prevent the_repository from coming back
> > @@ Commit message
> > coming back.
> >
> > Define the_repository to make it a compilation error so that they don't
> > - come back any more.
> > + come back any more; the repo parameter plumbed through the various
> > + functions can be used instead.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
>
> ... this piece of information.
Thanks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes since v2:
Changes since v1:
Remove explicit uses of the_repository and the_hash_algo from merge-ort, and since this has now been done multiple times for both merge-ort and replay, implement a small measure to prevent them from returning to either merge-ort or replay.
See https://lore.kernel.org/git/CABPp-BH7E1Bh2g0vR3T4NEsv34DvFQPzMuJSsqtOAaWY-fFCxg@mail.gmail.com/ and https://lore.kernel.org/git/CABPp-BFuwvqiCTCCpoyT6em9_1-qrgPWHWhrufQ3UuZ+Kfkb6A@mail.gmail.com/ for recent discussions on these.
Series overview:
Patches 1-4: Mostly mechanical removal of existing uses
Patches 5-6: Simple hammer to prevent the problem from returning
cc: "Kristoffer Haugsbakk" kristofferhaugsbakk@fastmail.com
cc: Patrick Steinhardt ps@pks.im
cc: Elijah Newren newren@gmail.com